273 patch partitioningspartidparents #302

ABLL526 · 2024-11-13T14:01:50Z

Adding get Ancestors feature:

PATCH /partitionings/{partId}/parents PATCH /partitionings/{partId}/parents #273
Removed 'Parent' wording to 'Ancestors' since there can be multiple parents going back 'generations'.
Similar to getFlowPartitionings
Any feedback, concern and questions are encouraged.

Feature 273 First Commit by Liam Leibrandt

Updated commit, updated tests.

ABLL526 · 2024-11-13T14:06:06Z

database/README.md

Can be ignored.
For my use.

ABLL526 · 2024-11-13T14:07:02Z

project/plugins.sbt

Can be ignored.
For my use.

What is it good for?
If for private use, don't put it into the repository.

Amended get ancestors functionality.

salamonpavel · 2024-11-29T10:57:04Z

database/README.md

@@ -32,3 +32,10 @@ to remove them or
 sbt flywayBaseline 
 ```
 to set the current state as the baseline.
+


This exact information is already in the readme file. Remove it.

Thanks @salamonpavel will remove the file from the PR.

salamonpavel · 2024-11-29T11:02:43Z

server/src/main/scala/za/co/absa/atum/server/api/http/Endpoints.scala

+    PartitioningWithIdDTO
+  ], Any] = {
+    apiV2.get
+      .in(V2Paths.Partitionings / path[Long]("partitioningId") / V2Paths.Partitionings)


the path is very likely incorrect ...
according to this design #141 it should be /partitionings/{id}/parents
potentially we can thinks of using 'ancestors' instead

You are correct, again I think I was doing some testing. I will change it as necessary.

the path is very likely incorrect ... according to this design #141 it should be /partitionings/{id}/parents potentially we can thinks of using 'ancestors' instead

ancestors is better IMHO.

Yeah why not, I'm okay with it. ALthough, potentially consider writing something short about it here in our Vocabulary documentation section: https://github.com/AbsaOSS/atum-service?tab=readme-ov-file#Vocabulary

salamonpavel · 2024-11-29T11:06:51Z

server/src/main/scala/za/co/absa/atum/server/api/http/Endpoints.scala

+  ], Any] = {
+    apiV2.get
+      .in(V2Paths.Partitionings / path[Long]("partitioningId") / V2Paths.Parents / path[Long]("partitioningId") / V2Paths.Parents )
+      .in(query[String]("byUser"))


there should be no query parameters, only request body ...
put the user into the body of the request

Understood, will fix.

salamonpavel · 2024-11-29T11:07:59Z

server/src/main/scala/za/co/absa/atum/server/api/http/Endpoints.scala

+    ParentPatchV2DTO
+  ], Any] = {
+    apiV2.get
+      .in(V2Paths.Partitionings / path[Long]("partitioningId") / V2Paths.Parents / path[Long]("partitioningId") / V2Paths.Parents )


incorrect path

You are correct, I think I was doing some testing. Will change to the correct path.

database/src/test/scala/za/co/absa/atum/database/runs/UpdateParentIntegrationTests.scala

...e/src/test/scala/za/co/absa/atum/database/runs/GetPartitioningMainFlowIntegrationTests.scala

salamonpavel · 2024-11-29T11:15:56Z

project/plugins.sbt

@@ -33,7 +33,7 @@ addSbtPlugin("com.eed3si9n" % "sbt-assembly" % "2.2.0")
 lazy val ow2Version = "9.5"
 lazy val jacocoVersion = "0.8.11-absa.1"

-def jacocoUrl(artifactName: String): String = s"https://github.com/AbsaOSS/jacoco/releases/download/$jacocoVersion/org.jacoco.$artifactName-$jacocoVersion.jar"
+def jacocoUrl(artifactName: String): String = s"file:///C:/Users/ABLL526/Documents/GitHub/Jar-Store2/org.jacoco.$artifactName-$jacocoVersion.jar"


Yes, this was for my use only. Will remove the file from the PR.

salamonpavel · 2024-11-29T11:16:07Z

project/plugins.sbt

@@ -46,4 +46,4 @@ addSbtPlugin("org.ow2.asm" % "asm" % ow2Version from ow2Url("asm"))
 addSbtPlugin("org.ow2.asm" % "asm-commons" % ow2Version from ow2Url("asm-commons"))
 addSbtPlugin("org.ow2.asm" % "asm-tree" % ow2Version from ow2Url("asm-tree"))

-addSbtPlugin("za.co.absa.sbt" % "sbt-jacoco" % "3.4.1-absa.4" from "https://github.com/AbsaOSS/sbt-jacoco/releases/download/3.4.1-absa.4/sbt-jacoco-3.4.1-absa.4.jar")
+addSbtPlugin("za.co.absa.sbt" % "sbt-jacoco" % "3.4.1-absa.4" from "file:///C:/Users/ABLL526/Documents/GitHub/Jar-Store2/sbt-jacoco-3.4.1-absa.3.jar")


Yes, this was for my use only. Will remove the file from the PR.

database/src/test/scala/za/co/absa/atum/database/runs/GetAncestorsIntegrationTests.scala

database/src/main/postgres/runs/V0.3.0.1__get_ancestors.sql

salamonpavel · 2024-11-29T12:31:23Z

database/src/main/postgres/runs/V0.3.0.2__patch_partitioning_parent.sql

+    PERFORM 1 FROM runs.partitionings WHERE id_partitioning = i_parent_id;
+    IF NOT FOUND THEN
+        status := 41;
+        status_text := 'Parent Partitioning not found';


Same here, we could distinguish was was not found.

salamonpavel · 2024-11-29T12:42:14Z

database/src/main/postgres/runs/V0.3.0.2__patch_partitioning_parent.sql

+            AND fk_flow != mainFlow
+    );
+
+    FOREACH var IN ARRAY flow_id LOOP


loops could be removed

salamonpavel · 2024-11-29T12:45:04Z

database/src/main/postgres/runs/V0.3.0.2__patch_partitioning_parent.sql

+ * limitations under the License.
+ */
+
+CREATE OR REPLACE FUNCTION runs.patch_partitioning_parent(


I am not sure about what the function does and is supposed to do. I thought we should check that partitioning and parent partitioning exists and then simply add the relationship (if not already there) using _add_to_parent_flows.sql ... @benedeki @lsulak

The function should set the relationship between the two partitions, considering the conditions are set. And based on input parameter(s) copy the metadata from parent to child.
Conditions to check:

partition exists

parent partition exists

the relationship does not exist already (then it's not an error, just no-op)

a reverse relationship does not exist
Metadata to copy, if input parameters says so:

measures

additional data

salamonpavel · 2024-11-29T12:54:33Z

server/src/main/scala/za/co/absa/atum/server/api/controller/PartitioningController.scala

+     partitioningId: Long,
+     parentPartitioningID: Long,
+     byUser: String
+ ): IO[ErrorResponse, SingleSuccessResponse[ParentPatchV2DTO]]


I am not sure about the return value. Usually PATCH returns updated resource. In our case though we edit/establish relationship in the db. I wonder if instead we should return 200 OK with partitioning and all its parent partitionings or return 200 Ok without data or 204 No Content.

I am for 200 Ok without any data, or 204 No Content.

the Id makes no sense

the partitioning doesn't give much value

and partitioning with all parents while occasionally useful could be taxing on the DB - better to request explicitly for those rare occasions

server/src/main/scala/za/co/absa/atum/server/api/database/runs/functions/GetAncestors.scala

server/src/main/scala/za/co/absa/atum/server/api/http/Routes.scala

database/src/main/postgres/runs/V0.3.0.1__get_ancestors.sql

benedeki · 2024-12-03T16:26:06Z

database/src/main/postgres/runs/V0.3.0.2__patch_partitioning_parent.sql

+    flow_id := array(
+            SELECT fk_flow AS flow_id
+            FROM flows.partitioning_to_flow
+            WHERE fk_partitioning = i_partitioning_id
+            AND fk_flow != mainFlow
+    );
+
+    FOREACH var IN ARRAY flow_id LOOP


Work with Arrays in PG is nto very effective. It's discouraged to operate on them, only use them as input or output.
Particularly bad is to do a FOR loop over them.

database/src/test/scala/za/co/absa/atum/database/runs/UpdateParentIntegrationTests.scala

benedeki · 2024-12-03T16:35:56Z

project/plugins.sbt

What is it good for?
If for private use, don't put it into the repository.

benedeki · 2024-12-03T16:37:22Z

server/src/main/scala/za/co/absa/atum/server/api/controller/PartitioningController.scala

+     partitioningId: Long,
+     parentPartitioningID: Long,
+     byUser: String
+ ): IO[ErrorResponse, SingleSuccessResponse[ParentPatchV2DTO]]


I am for 200 Ok without any data, or 204 No Content.

the Id makes no sense

the partitioning doesn't give much value

and partitioning with all parents while occasionally useful could be taxing on the DB - better to request explicitly for those rare occasions

benedeki · 2024-12-03T16:41:39Z

server/src/main/scala/za/co/absa/atum/server/api/http/Endpoints.scala

+    PartitioningWithIdDTO
+  ], Any] = {
+    apiV2.get
+      .in(V2Paths.Partitionings / path[Long]("partitioningId") / V2Paths.Partitionings)


the path is very likely incorrect ... according to this design #141 it should be /partitionings/{id}/parents potentially we can thinks of using 'ancestors' instead

ancestors is better IMHO.

lsulak · 2024-12-04T16:26:06Z

server/src/main/scala/za/co/absa/atum/server/api/controller/PartitioningController.scala

@@ -32,6 +32,12 @@ trait PartitioningController {
    partitioningSubmitDTO: PartitioningSubmitV2DTO
  ): IO[ErrorResponse, (SingleSuccessResponse[PartitioningWithIdDTO], String)]

+  def patchPartitioningParentV2(


My suggestion: try to create smaller PRs. In this case, all this could be split into 2 PRs:

SQL stuff only, with Balta tests that would test proposed DB functions

Scala part related to the backend - controller, service, repository, scala classes related to DB function wrapping/calls, etc

Also, not only splitting, but approaching this as a 2 step process can save you time, i.e. no need to implement the second part if your DB functions are not yet working or aren't satisfying API and/or functionality completely

.../main/scala/za/co/absa/atum/server/api/database/runs/functions/PatchPartitioningParent.scala

lsulak · 2024-12-04T16:33:58Z

server/src/main/scala/za/co/absa/atum/server/api/http/Endpoints.scala

+  : PublicEndpoint[(Long, Long, String), ErrorResponse, SingleSuccessResponse[
+    ParentPatchV2DTO
+  ], Any] = {
+    apiV2.get


also should be patch right? Not get

server/src/main/scala/za/co/absa/atum/server/api/http/Routes.scala

lsulak · 2024-12-04T16:46:54Z

.../test/scala/za/co/absa/atum/server/api/http/PatchPartitioningParentV2EndpointUnitTests.scala

+package za.co.absa.atum.server.api.http
+
+import org.mockito.Mockito.{mock, when}
+import sttp.client3.circe.asJson


few unused imports below

This is specifically for the database portion. 1. Changed functionality to simply add a parent. 2. Can copy the Measurements from parent. 3. Can copy the Additional Data from parent.

This is specifically for the database portion. 1. Made the necessary changes as mentioned by the team.

1. Made the necessary changes as mentioned by the team. 2. Made the necessary changes to the add parent Database functionality. 3. Made the necessary changes to the get ancestors Database functionality. 4. Made the necessary changes to the get ancestors Scala implementation. I still need to add the add parents Scala functionality.

ABLL526 added 3 commits November 12, 2024 15:48

Feature 273 First Commit

45e7578

Feature 273 First Commit by Liam Leibrandt

Merge branch 'master' into 273-patch-partitioningspartidparents

65ba7c9

Updated Commit

97c4581

Updated commit, updated tests.

ABLL526 added enhancement New feature or request good first issue Good for newcomers DB Issues touching the Database part of the project Server Issues touching the server part of the project labels Nov 13, 2024

ABLL526 added this to the v0.4.0 milestone Nov 13, 2024

ABLL526 self-assigned this Nov 13, 2024

ABLL526 requested review from benedeki, lsulak, TebaleloS, Zejnilovic, dk1844 and salamonpavel as code owners November 13, 2024 14:01

ABLL526 linked an issue Nov 13, 2024 that may be closed by this pull request

PATCH /partitionings/{partId}/parents #273

Open

ABLL526 commented Nov 13, 2024

View reviewed changes

database/README.md

Copy link

Author

ABLL526 Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be ignored.
For my use.

ABLL526 commented Nov 13, 2024

View reviewed changes

Added the patch parents functionality.

e89a330

Amended get ancestors functionality.

ABLL526 linked an issue Nov 28, 2024 that may be closed by this pull request

GET /partitionings/{partId}/parents -> returns all ancestors, not just direct ones #305

Open